Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Local/Built-in image proxy #1126

Merged

Conversation

DEVTomatoCake
Copy link
Member

@DEVTomatoCake DEVTomatoCake commented Jun 22, 2024

Currently, there's support for the Imagor image proxy. However, not everyone might be able to run or install it everywhere.

This PR adds a local image proxy which can be configured using the cdn_imagorServerUrl config. Currently, it has to be configured as <Base instance URL>/imageproxy.

  1. Hash is supported so external users cannot use the proxy with different URLs
  2. URL format is compatible with Imagor/Thumbor
  3. Resizing is supported using the following fallbacks:
    1. Try sharp (requires C binaries for build)
    2. Try jimp (pure JS, added as optional dependency)
    3. Nothing, image is returned as-is

Discussion

(Doesn't have to be on GitHub, Spacebar/Discord is also possible.)

Code

  • Can those @ts-expect-errors be fixed in a better way?
  • Re: ^, currently, if jimp is installed, L153 is fine with the @ts-expect-error comment. If it isn't, TS complains about the directive being unused. How can this be fixed?

Concept

  • I've currently limited the maximum Content-Length value to 10 MiB to prevent big files from overloading the server. Does this make sense? Is it better to, instead of waiting for the connection to finish, check the headers once they're received and instantly cancel the connection? Or does this not matter at all, and the limit should be removed completely?
  • In which cases would the proxy be enabled after all? Should it be enabled only if cdn_imagorServerUrl includes /imageproxy?

Config

  • Which kind of configs would be required for this? Suggestions:
    • Maximum file proxy size
    • Proxy timeout (currently set to 5 seconds)
    • Cache-Control header client cache duration

@DEVTomatoCake DEVTomatoCake marked this pull request as ready for review June 28, 2024 08:21
@TheArcaneBrony TheArcaneBrony self-requested a review as a code owner June 28, 2024 10:13
@MaddyUnderStars
Copy link
Member

I don't believe that this is in scope for spacebar, and it has additional performance considerations. Using Imagor makes more sense to me, as it's a dedicated tool written in a more performant language.

@MaddyUnderStars
Copy link
Member

You don't need to close it if you want to wait for what the others have to say. @Puyodead1 @TheArcaneBrony

@DEVTomatoCake DEVTomatoCake reopened this Jul 4, 2024
@TheArcaneBrony
Copy link
Member

I don't believe this is out of scope for Spacebar? Besides, I don't think performance considerations are that important. For those looking for better performance, Imagor seems to remain available as an option?

  • Rory

@DEVTomatoCake
Copy link
Member Author

Imagor seems to remain available as an option?

Yes, it should still be possible to use Imagor and that should be preferred too, but this PR helps if you cannot use Imagor yet.

@Puyodead1
Copy link
Contributor

honestly idfk, it seems like it could be useful

@MaddyUnderStars
Copy link
Member

And I've been outvoted! @DEVTomatoCake If you can fix the conflicts, I'll merge it

@MaddyUnderStars MaddyUnderStars merged commit dbd93bd into spacebarchat:master Aug 19, 2024
3 checks passed
@DEVTomatoCake DEVTomatoCake deleted the feat/local-image-proxy branch August 19, 2024 14:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants